Skip to content

DAOS-19526 dfs: Enable Hardlink support for dfuse - #19065

Open
sherintg wants to merge 6 commits into
feature/dfs_hardlinksfrom
sherintg/dfs_hardlinks/DAOS-19526
Open

sherintg wants to merge 6 commits into
feature/dfs_hardlinksfrom
sherintg/dfs_hardlinks/DAOS-19526

Conversation

@sherintg

Copy link
Copy Markdown
Collaborator

This commit implements the following changes:

  1. Introduced a callback handler for hardlink (link) in dfuse.
  2. Modified the dfuse unlink handler to treat the unlink of a hardlink differently from the removal of the last link.
  3. Introduced two new dfs functions for unlink and rename that tell the caller whether the target file was really removed/deleted. These apis are called by dfuse rename and unlink handlers.
  4. Added support for tracking multiple dentries per inode entry. The entries are updated as they are encountered during link, unlink, rename and lookup/readdir operations, and every name is invalidated when the inode is dropped.
  5. NLT functional dfuse tests and DFS unit tests for hardlinks.

Allow-unstable-test: true

Steps for the author:

  • Commit message follows the guidelines.
  • Appropriate Features or Test-tag pragmas were used.
  • Appropriate Functional Test Stages were run.
  • At least two positive code reviews including at least one code owner from each category referenced in the PR.
  • Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

  • Gatekeeper requested (daos-gatekeeper added as a reviewer).

@github-actions

github-actions Bot commented Sep 15, 2026 •

Copy link
Copy Markdown

Ticket title is ' Enable Hardlink support for dfuse'
Status is 'In Review'
https://daosio.atlassian.net/browse/DAOS-19526

@daosbuild3

Copy link
Copy Markdown
Collaborator

@sherintg
sherintg force-pushed the sherintg/dfs_hardlinks/DAOS-19526 branch from 14c35b2 to aff5ff5 Compare September 15, 2026 10:36
@daosbuild3

Copy link
Copy Markdown
Collaborator

@daosbuild3

Copy link
Copy Markdown
Collaborator

This commit implements the following changes:
1. Introduced a callback handler for hardlink (link) in dfuse.
2. Modified the dfuse unlink handler to treat the unlink of a hardlink
   differently from the removal of the last link.
3. Introduced two new dfs functions for unlink and rename that tell the
   caller whether the target file was really removed/deleted. These apis
   are called by dfuse rename and unlink handlers.
4. Added support for tracking multiple dentries per inode entry. The
   entries are updated as they are encountered during link, unlink,
   rename and lookup/readdir operations, and every name is invalidated
   when the inode is dropped.
5. NLT functional dfuse tests and DFS unit tests for hardlinks.

Allow-unstable-test: true

Signed-off-by: Sherin T George <sherin-t.george@hpe.com>
@sherintg
sherintg force-pushed the sherintg/dfs_hardlinks/DAOS-19526 branch from aff5ff5 to d0d244e Compare September 15, 2026 11:31
@sherintg
sherintg marked this pull request as ready for review September 15, 2026 12:30
@sherintg
sherintg requested review from a team as code owners September 15, 2026 12:30
@daosbuild3

Copy link
Copy Markdown
Collaborator

@mchaarawi mchaarawi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's going to take me a while to review this large PR.
for now, please checkout the copilot review in:
/home/chaarawi/reviews/PR-19065-dfuse-hardlinks-review.md
this is in the nfs from any of our internal hsw / brd nodes. let me know if you can't access it.

@mchaarawi

Copy link
Copy Markdown
Contributor

also please update the feature branch to latest master and merge this PR.
the branch is quite stale now.

@daosbuild3

Copy link
Copy Markdown
Collaborator

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-19065/3/testReport/

…9526

Signed-off-by: Sherin T George <sherin-t.george@hpe.com>
@sherintg

Copy link
Copy Markdown
Collaborator Author

@mchaarawi @kanard38 I have addressed the review comments including the merge.

@sherintg
sherintg requested a review from mchaarawi September 25, 2026 05:46
@sherintg
sherintg force-pushed the sherintg/dfs_hardlinks/DAOS-19526 branch from 7ed58df to 5f7332a Compare September 25, 2026 05:53
@mchaarawi

Copy link
Copy Markdown
Contributor

can you please repush with commit pragma:

Features: dfs dfuse pil4dfs

@mchaarawi

Copy link
Copy Markdown
Contributor

a few nits:

  1. the last commit message looks inaccurate:
  • dfs: set the 'deleted' out-param in dfs_move_internal() when a rename
    resolves the same object id

dfs_move_internal looks untouched to me. The actual DFS change is the remove_hardlink() restart fix.

  1. there is no EXDEV test for the new check in df_ll_link(). so that has no test coverage now.
    should be simple to add a pool-level mount (DFuse(..., container=None)) with two containers and os.link across them expecting errno.EXDEV

  2. couple things to address and require tickets to follow on:

  • ln -P symlink hl (hardlink to a symlink)
  • pil4dfs interception

Comment thread Jenkinsfile
test_script: 'ci/unit/test_nlt.sh' +
' --system-ram-reserved 4' +
' --max-log-size 1950MiB' +
' --max-log-size 2500MiB' +

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new set of tests added, I had seen failure related to log-size not sufficient. Hence increased the size.

…link

Rework for the dfuse hardlink support.

- dfs: in remove_hardlink(), set the 'deleted' out-param on the branch that
  drops the final link (link_cnt reaches 0), so the value is set on every
  path and stays correct across a transaction restart instead of retaining
  a stale result from an earlier attempt; previously it was only cleared to
  false when a link survived.

- dfuse/unlink: evict the surviving inode's metadata cache in
  dfuse_hardlink_removed() so a peer-name unlink/rename no longer leaves
  stale st_nlink/ctime for the kernel's follow-up GETATTR to read.

- dfuse/fuseops: reject cross-container hardlinks in df_ll_link() with
  EXDEV before calling into DFS, instead of relying on an oid lookup that
  fails with a misleading ENOENT (oids are container-scoped).

- dfuse/core: in dfuse_ie_dentry_replace(), de-duplicate when the rename
  destination name is already tracked (primary or secondary) so the
  secondary list never gains a duplicate entry.  Defer freeing a removed
  dentry in both dfuse_ie_dentry_replace() and dfuse_ie_dentry_set_single()
  until after the spinlock is released, matching the other dentry helpers.

- tests: add test_hardlink_stale_notify_delete (out-of-band removal then a
  local final unlink to exercise notify_delete on stale same-parent
  dentries), add test_hardlink_stale_name, add test_hardlink_cross_container
  (pool-level mount with two containers, os.link across them expects EXDEV),
  and assert the refreshed link count after a peer-name unlink in
  test_hardlink_caching.

Features: dfs dfuse pil4dfs
Allow-unstable-test: true

Signed-off-by: Sherin T George <sherin-t.george@hpe.com>
@sherintg
sherintg force-pushed the sherintg/dfs_hardlinks/DAOS-19526 branch from 5f7332a to 3b0b5ab Compare September 25, 2026 14:45
@daosbuild3

Copy link
Copy Markdown
Collaborator

@sherintg

Copy link
Copy Markdown
Collaborator Author

@mchaarawi I have incorporated the comments.

  1. I have added additional test for cross container link() op.
  2. repushed with the amended commit string.
  3. Created a ticket DAOS-19701 for pil4dfs changes.
  4. hardlinks on symlinks is not supported by the current implementation. As discussed over chat, I will add it as limitation in the user documentation.

Addressed a regression seen in CI/CD where open_stat() in DFS was not
sanitizing the mode passed as argument. The fix strips the internal bits
(DFS_EXTERNAL_MODE) from caller-supplied modes at the DFS boundary so an
untrusted mode can never forge internal state:
- open_stat(): mask the mode before it is stored; legitimate hardlinks are
  still flagged from fetched entry state after creation.
- dfs_mkdir(): mask the mode before building the directory entry.
- dfs_osetattr(): return the sanitized external mode in the out stat instead
  of echoing the caller's raw mode.

Features: dfs dfuse pil4dfs
Allow-unstable-test: true

Signed-off-by: Sherin T George <sherin-t.george@hpe.com>
@daosbuild3

Copy link
Copy Markdown
Collaborator

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-19065/7/testReport/

Addressed a regression seen in CI/CD where simul tests expected hardlink
related tests to fail. Now hardlink related tests are removed from the
faillist.

Features: dfs dfuse pil4dfs
Allow-unstable-test: true

Signed-off-by: Sherin T George <sherin-t.george@hpe.com>
@sherintg
sherintg requested review from a team as code owners September 28, 2026 05:56

@knard38 knard38 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First round of the comment

Comment thread src/client/dfuse/dfuse.h
dfuse_ie_dentry_inval(struct dfuse_info *dfuse_info, struct dfuse_dentry *released);

/* Queue every name in released for invalidation on the invalidation thread, consuming released.
* ie_drop, if set, is released after the final name is invalidated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be worth of it to mention that there is some actions which are always taken even in case of error.
For example removing the entries and calling dfuse_inode_decref()

Comment thread src/client/dfuse/inval.c
}

void
dfuse_ie_inode_delete(struct dfuse_info *dfuse_info, struct dfuse_inode_entry *ie,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to reuse the  dfuse_queue_inval_dentries() /  ival_drain_queue()  pattern here, so that the fuse_lowlevel_notify_delete() calls are issued from the invalidation thread rather than synchronously on the FUSE worker?

From my investigation with copilot, in the kernel, fuse_reverse_inval_entry() takes the parent directory's i_rwsem , which may be held by another in-flight operation itself waiting for a dfuse reply. With several stale names in other directories this could stall the worker noticeably and reduce the reactivity of the dfuse daemon.
Delaying the delete should be safe since the kernel checks the child nodeid before d_delete(). Adding an ino field to struct dfuse_inval_item plus a branch in ival_drain_queue() looks sufficient.

From my side, it is a nice to have but could be worth considering for a follow-up PR.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@daltonbohning daltonbohning left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ftest changes LGTM

@mchaarawi

Copy link
Copy Markdown
Contributor

2 issues found. use reproducer: /home/chaarawi/reviews/pr19065-repro.c

  • Creating a hardlink does not invalidate the destination’s directory cache: New link callback never calls dfuse_cache_evict_dir(). Neither its wrapper nor dfuse_reply_entry() does so. With caching enabled, an open directory reader can retain the shared readdir cache. After link(), a new reader reuses that still-valid cache and can omit the new name. Create, unlink, and rename already invalidate this cache. Add the same invalidation here. Missing test: prime and retain a directory stream, create a hardlink, then verify an independently opened stream includes it.
  • The link reply can move cached mtime backward: The callback publishes dfs_link()’s returned attributes directly. However, that stat construction takes mtime from stored inode metadata and ignores the array’s modification epoch. Normal DFS stat reconciles both through update_stbuf_times(). With attribute caching enabled and writeback disabled, write and close a, record its modification time, then link(a,b). The link response can replace the shared inode’s cached modification time with the older stored value, so immediate stat calls report the wrong time until refresh. The underlying DFS discrepancy predates this PR, but this new callback exposes it through dfuse’s attribute cache. Reuse the normal timestamp calculation and add a before/after-link timestamp assertion.
mkdir -p /tmp/chaarawi/pr19065-off-20260930 /tmp/chaarawi/pr19065-cache-20260930

/** NO caching (will pass) */
daos container set-attr mypool mycont2 \
  dfuse-data-cache:off,dfuse-attr-time:0,dfuse-dentry-time:0,dfuse-ndentry-time:0

dfuse --disable-caching --disable-wb-cache -t 4 -e 1 /tmp/chaarawi/pr19065-off-20260930 mypool mycont2
~/reviews/pr19065-repro /tmp/chaarawi/pr19065-off-20260930

/** with caching (will fail) */
daos container set-attr mypool mycont2 \
  dfuse-data-cache:off,dfuse-attr-time:60,dfuse-dentry-time:60,dfuse-ndentry-time:60

dfuse --enable-caching --disable-wb-cache -t 4 -e 1 \
  /tmp/chaarawi/pr19065-cache-20260930 mypool mycont2

~/reviews/pr19065-repro \
  /tmp/chaarawi/pr19065-cache-20260930 \
  /tmp/chaarawi/pr19065-off-20260930

@sherintg
sherintg requested review from knard38 and mchaarawi October 1, 2026 08:18
@sherintg

sherintg commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator Author

@mchaarawi @knard38 I have addressed the review comments. pls check.

Rework addressing review feedback on the hardlink support.

- dfs_link() built the returned stat directly from the stored inode
  entry, whose mtime lags the array's modification epoch.  Route it
  through update_stbuf_times() like every other stat path so the
  reconciled mtime is returned.  Without this, dfuse's attribute cache
  is poisoned with the stale value and an immediate stat after link()
  reports the modification time moving backwards.

- dfuse_cb_link() never evicted the destination directory's cache, so a
  retained readdir stream kept dfuse's shared readdir handle resident
  and an independently opened stream was served a stale listing that
  omitted the new name.  Evict it, matching create/unlink/rename.

- dfuse_ie_inode_delete() issued fuse_lowlevel_notify_delete()
  synchronously on the FUSE worker pool.  That call blocks acquiring the
  parent's kernel i_rwsem, which may be held by another in-flight
  operation waiting on the same pool, stalling the worker.  Queue the
  deletes on the invalidation thread instead, extending struct
  dfuse_inval_item with an 'ino' and a 'delete_entry' flag.  Delaying is
  safe because the kernel matches the child nodeid before deleting, so a
  name re-created in the meantime is left untouched.

Added NLT tests test_hardlink_dir_cache and test_hardlink_mtime, which
fail on the unfixed code and pass with these changes.

Features: dfs dfuse pil4dfs
Allow-unstable-test: true

Signed-off-by: Sherin T George <sherin-t.george@hpe.com>
@sherintg
sherintg force-pushed the sherintg/dfs_hardlinks/DAOS-19526 branch from 76d94bb to 3a10601 Compare October 1, 2026 08:59

@knard38 knard38 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for what I understand

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants